-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Client refactor #658
Client refactor #658
Conversation
- Remove `Func` from `MustDoFunc` and `DoFunc`. They were called that to differentiate from the old-style `Do` and `MustDo` which are no more. - Move sync functions to a new file. - Move registration/login functions to a new file. - Move `/internal/client` to `/client`.
Also fix bad merge conflict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from comments, LGTM
// Unsafe_SendEventUnsynced sends `e` into the room. This function is UNSAFE as it does not wait | ||
// for the event to be fully processed. This can cause flakey tests. Prefer `SendEventSynced`. | ||
// Returns the event ID of the sent event. | ||
func (c *CSAPI) Unsafe_SendEventUnsynced(t TestLike, roomID string, e b.Event) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (c *CSAPI) Unsafe_SendEventUnsynced(t TestLike, roomID string, e b.Event) string { | |
func (c *CSAPI) UnsafeSendEventUnsynced(t TestLike, roomID string, e b.Event) string { |
Maybe? (same for the others)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to keep the _
to make it very clear that it's UNSAFE_SomeFuncName`, rather than just the function happens to have "unsafe" in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should jump out at the reader as being dodgey is my point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(otherwise, no objections)
Func
fromMustDoFunc
andDoFunc
. They were called that to differentiate from the old-styleDo
andMustDo
which are no more./internal/client
to/client
.TestLike
, an interface to representtesting.T
to allow the client to be used outside of tests e.g benchmarks.Slightly not client related:
federation.Event
instead of abusingb.Event
for creating events over federation. Makes this much clearer and fixes Add client.Event #381Fixes #526 , #381
Partly addresses #226